I'm tempted to merge this and cut a release because it does fix the issue.

But I'm afraid this is more complexity than desired.. is the ref to the handler really needed? Maybe we need to change things on the @react-navigation/core side, but I feel that useEffect should be sufficient for this use case

Yes it is necessary because otherwise if the key does not change, the useEffect closure will capture the initial handleEvt fn, and that initial fn will be called by the navigation listeners. We really want to ensure that the last handleEvt fn provided by the user is called.

Let's consider that the key will not change.
If user does useNavigationEvent(bool ? handler1: handler2), only one of the handler will keep being called because it was captured in the useEffect closure, even if the bool value change over time, which is dangerous and unexpected from an user point of view.

The recommended way is to unsub/resub as it's generally fast and does not produce side effects.
https://twitter.com/dan_abramov/status/1066374847427682305

However for react-navigation, on (re)subscribe you might get a "focus" event firing synchronously. So if we want to simplify this implementation, we must remove that react-navigation-core behavior first, which may be a wanted. I don't know the details of why we need that event firing synchrously on subscribe but you or @brentvatne are likely the best to know that ;)

However for react-navigation, on (re)subscribe you might get a "focus" event firing synchronously.

That sounds like the root of the problem that could be solved. Ideally resubscription shouldn't trigger side effects.

With the approach in this PR, I don't understand why [navigation.state.key] is the second argument. If you wanted a stable listener, why not just [] and always use the same one? Otherwise you'd get the same "side effects on resubscription" problems when navigation.state.key changes. Or is that desirable?

Another minor caveat is that useEffect has been reported to be buggy in RN and fired way too late. Don't know if that got fixed.

Thanks for your review @gaearon

That sounds like the root of the problem that could be solved. Ideally resubscription shouldn't trigger side effects.

Agree with that, but I suspect there's a good reason to have that side effect that I'm not aware of.

[navigation.state.key] is the inputs because we actually want to resubscribe when the navigation state change. Not sure to exactly know why and @ericvicenti probably knows this better, but I suspect this means the navigation passed through props is not the same as before (ie, a different listener list). The key is generally stable during the lifecycle of a react-navigation screen and should change on screen replace.

I wonder if [] could not be used instead, because I suspect react-navigation does remount the screen component on state.key change (can someone confirm?). If the key can't change over the lifecycle of a screen comp, there's probably no need to use it as input.

Yes I've seen useEffect is not working properly (just released https://github.com/slorber/react-native-animation-hooks which has the useEffect issue), but hopefully this implementation is supposed to work when useEffect will work :)

Thanks for the advice @gaearon!

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

When we call addEventListener, it starts listening to the events of the current route, which is identified as navigation.state.key. In almost all circumstances, it will never change. But, if it does change for some reason, it would make sense for our listener to unsubscribe and re-subscribe to the new route. Which is why I think [navigation.state.key] makes sense as the second argument.

(When I first tried using the second argument with state.key, I think I saw some bad and confusing behavior. But I can't really remember what was going wrong, so if this second argument works for anyone else, lets just move forward with it until a more specific bug emerges)

I prototyped this stuff on web, so I haven't had too much time with hooks on RN. Once the useEffect thing is sorted out, I'll move over my RN project to use ReactNavHooks

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

I don't personally have any knowledge about resubscription behavior. I've never run this code — I'm here from a twitter thread with @slorber.

I was referring to this comment (#8 (comment)) which says that it causes focus to fire.

Yeah I figured you had been summoned here from Twitter 😂👋 It was a poorly-addressed "you're".

@slorber, let me know what happens in your code when you try putting the route key in the second argument of useEffect. Hopefully it will work fine!

@ericvicenti if I put the route key in my impl (which I did in this PR) it works fine, much better than the current implementation. But I encounter another weird bug afterward where navigation events stop firing completely (as far as I remember, this is true for both your impl and mine, using your react navigation web example projet as reference, so it does not look like a regression)

I'm looking at the events code and I can't see how/why/when we would get a synchronous focus event when (re)subscribing. Unless you're referring to the actual normal focus event, which might be happening around the same time as the re-subscription? Do you have an easy way to repro this behavior?

Yes, I probably was wrong and this focus event is not totally synchronous but happens just after subscribe.

This behavior lead to infinite loops when using an arrow function (see react-navigation/react-navigation#5058)

<NavigationEvents onDidFocus={() => this.setState({})} />

The setState trigger a new render, and in the next update, as the closure might have changed, we have to unsub/resub, but we are still in the screen appearance phase, so the onDidFocus event keeps firing again and again as we unsub/resub.

Using a stable listener did not have that bug, because former implementation did not unsub/resub when the callback is stable.

The fix to always register a stable listener on mount was merged and fixes the issue:
react-navigation/core#17

My PR here is supposed to also ensure this issue does not happen with hooks when using an arrow function listener ;)

The user might to something like this which would trigger the same issue:

useNavigationEvents(event => {
  if ( event.type = "focus" ) setIsFocusedInState(true);
})

Hi @ericvicenti

The latest article from @gaearon actually explains the problem you had when using useEffect key and why my solution solves this problem.

https://overreacted.io/making-setinterval-declarative-with-react-hooks/

As far as I understand, if you really want to use a key, then you have no choice than using a ref to ensure the latest closure will be run.

So the implementation depends on weither you want to use a key or not :) I don't totally measure the impact of using a key honestly.

Any news on this topic? :)

Hi @benseitz

I'd like those issues to be fixed as well, and could fix the PR (migrate to TS) if its current impl is approved.

But I think we didn't really settle on how this should be fixed yet. Unfortunately I think @ericvicenti is the one who knows best this part of the lib and will be the best to take a decision but he looks busy working on his Aven project.

My last attempt to analyze the problem is here: react-navigation/rfcs#72 (comment)

Basically I think we should be able to unsub/resub easily without having side effects on resub.

Currently the following leads to counter=2, I think it should lead to counter=1. If this is solved in core, we could more easily implement a hook which would simply unsub/resub everytime.

componentDidMount() {
    this.props.navigation.addListener('willFocus', () => {
      this.incrementCounter();
      this.props.navigation.addListener('willFocus', () => {
        this.incrementCounter();
      });
    });
  }

Superseded by #38